Python: Start using the shared data flow libraries #3701

yoff · 2020-06-12T09:59:48Z

Pull the identical files into codeql/python/ql/src/semmle/code/python/dataflow, note that we do not currently have a directory called code under semmle, but the other users of the shared libraries use this location.
Implement enough stubs to have all files compile while keeping the identical files identical.
Record the Python components in codeql/config/identical-files.json.
Fill in the stubs with references to the PointsTo library
Test that we get desired data flows
Switch to new CFG implementation based on type tracking
Test that we get desired data flows

tausbn · 2020-06-12T11:13:59Z

Just a few quick thoughts (as you've somewhat preempted the issue I'm writing 🙂):

This should perhaps go in experimental until we've vetted it.
We'll probably want to set up some automatic synchronisation of these libraries with the main copy. this is what Go does.

aschackmull · 2020-06-12T11:19:32Z

We'll probably want to set up some automatic synchronisation of these libraries with the main copy. this is what Go does

Go needs a separate sync mechanism because it's in a separate repo. For python simply using the identical-files.json-mechanism should be fine (that's how C/C++, Java, and C# sync).

aschackmull · 2020-06-12T11:22:31Z

Pull the identical files into codeql/python/ql/src/semmle/code/python/dataflow, note that we do not currently have a directory called code under semmle, but the other users of the shared libraries use this location.

You can use whatever path prefix you want - you don't have to use the code dir if it doesn't match up with what you currently have.

yoff · 2020-06-12T12:41:35Z

This should perhaps go in experimental until we've vetted it.

Ah, I have yet to discover experimental, that sounds very appropriate :-)

Perhaps a not-python-specific version of this could go into the shared implementation.

yoff · 2020-06-15T14:58:12Z

@aschackmull thanks for the meeting. It has resulted in readme.md, feel free to have a quick look and see if I have misunderstood/misremembered anything.

also restore accidentally removed comment

intrigus-lgtm · 2020-06-16T09:19:57Z

I find all of this very interesting, so if you can, keep the documentation as detailed as it is now :)

yoff · 2020-06-16T12:53:44Z

@intrigus-lgtm Do you mean readme.md? I am not sure I have written much documentation otherwise (although I have stolen heaps from the other language implementations).

need to actually have `OutNode`s

intrigus-lgtm · 2020-06-16T15:44:46Z

@intrigus-lgtm Do you mean readme.md? I am not sure I have written much documentation otherwise (although I have stolen heaps from the other language implementations).

Yeah, readme.md and just looking at the separate commits that implement data flow step by step is interesting :)

also more grouping

Also, fix test for local flow

but I think they receive no flow

yoff · 2020-06-17T20:23:43Z

Yeah, readme.md and just looking at the separate commits that implement data flow step by step is interesting :)

Ok, I am afraid I have made some messy progress lately :-) I will try to add my more stable findings to the readme so there will be some sort of thread..

between SSA variables and control flow nodes

python/ql/src/experimental/dataflow/internal/DataFlowPublic.qll

python/ql/src/experimental/dataflow/internal/DataFlowPrivate.qll

yoff · 2020-06-30T20:04:32Z

As part of my attempt at implementing field flow, I did some refactoring in a recent commit to #3830.

I mainly got rid of Node::asCfgNode and Node::asVariable, replacing them with casts to (new) subclasses.

If this is an improvement, I can do the same here..

to receive updates from data flow library

to get updated dataflow files

RasmusWL · 2020-07-01T15:13:42Z

I mainly got rid of Node::asCfgNode and Node::asVariable, replacing them with casts to (new) subclasses.

yep, sounds like the right approach!

I haven't had the time to look everything over yet, but this one sounds like a win 👍

tausbn · 2020-07-02T12:15:10Z

I agree with RasmusWL's comment above. Splitting it out into separate classes is very reminiscent of how it's done in the JavaScript libraries, and (I think) greatly increases the readability.

RasmusWL

Nice! Have some minor suggestions, but otherwise this looks like a good first start 🎉

Some thoughts after reading through the PR:

getEnclosingCallable doesn't really fit the way Python programs are executed, since you can execute things in the module scope. I guess the most straightforward way of shoehorning Python into the existing framework would be to define a "fake" callable for the module scope.
ReturnKind might be useful for differentiating between the return values of normal functions, generators (using yield), and async function returns.

What happens with named arguments? What does C# do?

In C# using named arguments is always interchangeable with using positional arguments, so the situation is not quite the same. Notice that in Python these are called keyword arguments (as opposed to named arguments in C#). I have been digging into our handling of keyword arguments recently, so would be happy to chat about this 👍

python/ql/src/experimental/dataflow/internal/TaintTrackingPrivate.qll

python/ql/src/experimental/dataflow/internal/DataFlowPublic.qll

python/ql/src/experimental/dataflow/internal/DataFlowPrivate.qll

…ate.qll Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

RasmusWL

Nice! Looks good to me now 👍 (but let's get approval from Taus as well before merging)

The part about documentation for toString actually applied to multiple member-predicate definitions, I guess I should have called that out 😅

Lastly I have to admit that I didn't look too closely on the tests and expected output, since it seemed quite cumbersome. I realize I forgot to mention this, and if there is anything in particular you want me to look at, let me know :) Would be great if we could get some way to annotate the test code so it's easy to spot in the .expected files if anything is not working as we expect it to.

yoff · 2020-07-03T12:58:37Z

Ah, I thought I saw another toString and got worried that I had not pulled the merged suggestion, but then I could not find it again :-)

The tests should be annotated, yes, and some are perhaps more debug than tests at the moment.

It might be nice to have tests for all the classes of nodes in the interface, then we can quickly see if we change the meaning of those..

tausbn · 2020-07-03T12:59:27Z

I am happy to merge the PR as it is.

Regarding toString, I think we need go through all of our documentation at some point anyway, to ensure consistency.

Regarding test output, I also didn't study it in detail, but did a bit of random sampling here and there. I think improving the test setup for this should be a fairly high-priority item.

Python: Minimal compilation of shared dataflow

375da38

yoff added the Python label Jun 12, 2020

yoff added 3 commits June 15, 2020 08:01

Summary of recent meeting.

1af2e56

Perhaps a not-python-specific version of this could go into the shared implementation.

Python: Address QL docs

6dfb3a5

Python: start on local flow

f8eb583

yoff added 2 commits June 16, 2020 07:30

Python: group related predicates

ad04ec5

also restore accidentally removed comment

Python: bit more local flow and fix ql docs

0abba23

Python: small test of local flow

f3e879a

Python: small start on global flow

0f77403

need to actually have `OutNode`s

yoff added 10 commits June 17, 2020 06:46

Python: move shared dataflow to experimental

e192b66

Python: fix identical-files.json after move

47f5b04

also more grouping

Python: update paths after move

52898f1

Python: Implement OutNode

71f364e

Also, fix test for local flow

Python: refactor test for global flow

8e51b2f

Python: Implement parameter nodes

25d624d

Python: Implemented return node

a45b5a7

but I think they receive no flow

Python: add flow from ArgumentNodes

f24dc69

Python: Use CallableValue and improve tests

ce57a28

Python: more local flow and more tests

c20219c

yoff added 2 commits June 18, 2020 07:45

Python: implemented ParameterNode, updated test

d283919

Python: General comment on dataflow

1562f5c

between SSA variables and control flow nodes

aschackmull reviewed Jun 30, 2020

View reviewed changes

python/ql/src/experimental/dataflow/internal/DataFlowPublic.qll Outdated Show resolved Hide resolved

aschackmull reviewed Jun 30, 2020

View reviewed changes

python/ql/src/experimental/dataflow/internal/DataFlowPrivate.qll Outdated Show resolved Hide resolved

yoff added 8 commits July 1, 2020 07:15

Merge branch 'master' of github.com:github/codeql into SharedDataflow

e259281

to receive updates from data flow library

Python: sync dataflow library

3388ca4

Python: simplify according to review comments

825f24a

Python: make compile and simplify

7787900

Python: Enable consistency check (currently fails)

79e4f1e

Merge branch 'master' of github.com:github/codeql into SharedDataflow

7fb97d7

to get updated dataflow files

Sync dataflow files

0175d5b

Python: make compile

0b11e77

yoff added 4 commits July 2, 2020 16:20

Python: update TODO

5f18fb4

Python: refactor Node class

bdc68ce

Python: exclude global vars from local flow

a9e0288

Python: add some links to readme

e366600

RasmusWL requested changes Jul 3, 2020

View reviewed changes

yoff and others added 5 commits July 3, 2020 13:30

Update python/ql/src/experimental/dataflow/internal/TaintTrackingPriv…

40a6728

…ate.qll Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

Update python/ql/src/experimental/dataflow/internal/DataFlowPublic.qll

8891fbf

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

Update python/ql/src/experimental/dataflow/internal/DataFlowPublic.qll

59d611d

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

Update python/ql/src/experimental/dataflow/internal/DataFlowPrivate.qll

d201eb2

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

Python: Address review comments

33cf96c

RasmusWL previously approved these changes Jul 3, 2020

View reviewed changes

Python: correct doc for toString

fe9520b

yoff dismissed RasmusWL’s stale review via fe9520b July 3, 2020 13:05

RasmusWL approved these changes Jul 3, 2020

View reviewed changes

tausbn merged commit 01c4852 into github:master Jul 3, 2020

Python: Start using the shared data flow libraries #3701

Python: Start using the shared data flow libraries #3701

Uh oh!

Conversation

yoff commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tausbn commented Jun 12, 2020

Uh oh!

aschackmull commented Jun 12, 2020

Uh oh!

aschackmull commented Jun 12, 2020

Uh oh!

yoff commented Jun 12, 2020

Uh oh!

yoff commented Jun 15, 2020

Uh oh!

intrigus-lgtm commented Jun 16, 2020

Uh oh!

yoff commented Jun 16, 2020

Uh oh!

intrigus-lgtm commented Jun 16, 2020

Uh oh!

yoff commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yoff commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RasmusWL commented Jul 1, 2020

Uh oh!

tausbn commented Jul 2, 2020

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

yoff commented Jul 3, 2020

Uh oh!

tausbn commented Jul 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yoff commented Jun 12, 2020 •

edited

Loading

yoff commented Jun 17, 2020 •

edited

Loading

yoff commented Jun 30, 2020 •

edited

Loading